feat(workspace): add workspace resource model with scoping, membershi…#2243
feat(workspace): add workspace resource model with scoping, membershi…#2243derekwaynecarr wants to merge 22 commits into
Conversation
2468c04 to
77ef1fc
Compare
Thread workspace through compute drivers (Docker, Podman, K8s) with
converged container naming (openshell-{workspace}--{name}-{id}),
workspace labels, and label-based lookup. Fix sandbox-side policy sync
to use learned workspace instead of hardcoding empty string. Fix TUI
all-workspaces view to use per-row workspace for sandbox actions. Add
provider profile to workspace deletion blocking check. Thread workspace
through SSH config generation with workspace-qualified host aliases.
Signed-off-by: Derek Carr <decarr@redhat.com>
Add workspace as a required parameter for resource-scoped operations (create, get, delete, wait_ready, wait_deleted) and optional for list operations that support all_workspaces global lookup. Rename ClusterInferenceConfig to InferenceRouteConfig and set_cluster/get_cluster to set_route/get_route to match the renamed proto RPCs. Signed-off-by: Derek Carr <decarr@redhat.com>
77ef1fc to
7bba756
Compare
- Backfill provider_credential_refresh_state in workspace migration - Set requires_workspace() to true for refresh state - Reject consecutive hyphens in workspace and sandbox names - Restore parse_host rejection of consecutive hyphens in all segments - Pass workspace from SSH session metadata in put_if call - Require explicit workspace in Python SDK Sandbox() constructor - Add sandbox name character validation matching workspace rules Signed-off-by: Derek Carr <decarr@redhat.com>
- Add watch channel to broadcast workspace from policy poll loop to denial and activity flush tasks, fixing proposals targeting the wrong workspace for non-default sandboxes - Add set_workspace() to CachedOpenShellClient for pre-seeding workspace on fresh connections - Use selected_provider_workspace() in TUI provider get/delete actions instead of current_workspace, fixing cross-workspace misrouting Signed-off-by: Derek Carr <decarr@redhat.com>
Signed-off-by: Derek Carr <decarr@redhat.com>
…le scope binding Provider profiles can be global (platform-scoped) or workspace-scoped. Previously, profile lookups used the ambient request workspace, which caused pre-existing global profiles (workspace="") to become invisible after the workspace migration. Add a `profile_workspace` field to the Provider proto that explicitly declares where the provider's type profile is stored. The field must be empty (global) or match the provider's own workspace — cross-workspace references are rejected. All profile lookup call sites now use `provider.profile_workspace` instead of the ambient workspace. Existing serialized providers get `profile_workspace=""` via protobuf defaults, correctly pointing to their global profiles with no migration. CLI gains `--global-profile` flag on `provider create`. Signed-off-by: Derek Carr <decarr@redhat.com>
…-back handle_revoke_ssh_session passed empty string for workspace in put_if instead of session.object_workspace(), dropping the workspace on the revoked session record. Signed-off-by: Derek Carr <decarr@redhat.com>
The lint handler was passing an empty string to profile_conflict_diagnostics instead of using the request workspace, so it only checked for conflicts in global scope rather than the workspace the user specified. Signed-off-by: Derek Carr <decarr@redhat.com>
- Fix proto comments: ListProviderProfilesRequest.workspace no longer claims two-tier merged resolution, LintProviderProfilesRequest.workspace now documents that it is used for conflict detection - Converge Podman driver labels to openshell.ai/ prefix by importing from driver_utils, matching Docker and Kubernetes drivers - Replace debug_assert with runtime PersistenceError for workspace enforcement in put_message, put_scoped_message, and cas_update_message - Add comment in parse_host explaining -- delimiter safety invariant - Read OPENSHELL_WORKSPACE env var in tab completers instead of defaulting to empty string - Remove unused _workspace params from provider_refresh_defaults and dynamic_token_grant_bindings_for_provider - Remove scattered workspace "default" fallbacks in service_routing, inference, and compute; normalization is handled by resolve_workspace at the handler layer Signed-off-by: Derek Carr <decarr@redhat.com>
- Backfill policy and draft_chunk object types in migration - Add label selector support to all_workspaces sandbox listing - Replace per-member deletion loop with batch delete_all_in_workspace - Extract shared validate_dns1123_label helper from duplicated validation - Add count_in_workspace for efficient member cap enforcement - Document intentional omission of workspace filter in list_by_scope Signed-off-by: Derek Carr <decarr@redhat.com>
Fix workspace-scoped cleanup bugs: settings deletion used empty workspace instead of sandbox workspace, migration backfill missed sandbox_settings, SSH session cleanup scanned globally instead of by workspace, and workspace deletion did not check for blocking sandbox_settings records. Add K8s resource name length validation to reject names exceeding DNS-1123 63-char limit. Update RFC 0011 to match Phase 1 implementation: document inference RPC rename, UpdateProviderProfiles with optimistic concurrency, Provider profile_workspace field, Python SDK workspace requirements, service hostname breaking change, and persistence validation behavior. Signed-off-by: Derek Carr <decarr@redhat.com>
Fix bugs found during PR NVIDIA#2243 review: correct migration type strings, resolve platform-scoped profile workspace handling, fix sandbox workspace_tx single-fire, add missing workspace deletion blockers, reset TUI row indices on workspace switch, fix TUI provider update workspace, and update e2e Python tests for workspace and inference route API changes. Add Phase 2 TODO comments for authorization gaps. Fix pre-existing clippy errors (too_many_arguments, cast_sign_loss, iter_on_single_items). Signed-off-by: Derek Carr <decarr@redhat.com>
…ace args Add Rust handler test and Python e2e test verifying that platform-scoped and workspace-scoped provider profiles are isolated from each other. Fix missing workspace keyword arguments in test_sandbox_api.py and test_sandbox_providers.py that caused TypeErrors. Signed-off-by: Derek Carr <decarr@redhat.com>
Fix reconciler test that stored sandbox settings with workspace "" while the sandbox itself used "default", causing the cleanup delete to miss the record. Fix e2e tests missing the required workspace keyword argument on SandboxClient.create, .get, .delete, and .wait_deleted calls. Signed-off-by: Derek Carr <decarr@redhat.com>
Fix bugs found during PR NVIDIA#2243 review: correct migration type strings, resolve platform-scoped profile workspace handling, fix sandbox workspace_tx single-fire, add missing workspace deletion blockers, reset TUI row indices on workspace switch, fix TUI provider update workspace, and update e2e Python tests for workspace and inference route API changes. Add Phase 2 TODO comments for authorization gaps. Fix pre-existing clippy errors (too_many_arguments, cast_sign_loss, iter_on_single_items). Signed-off-by: Derek Carr <decarr@redhat.com>
…label limits Make sandbox owned-record cleanup fatal: policy revisions, draft chunks, SSH sessions, and settings must all be deleted before the sandbox itself is removed. Previously errors were swallowed as warnings, leaving orphan records that permanently blocked workspace deletion. Fix platform-profile attachment checks to scan sandboxes across all workspaces. Previously scan_sandboxes queried workspace="" which returned zero results since sandboxes always live in named workspaces. Platform profile updates and deletions now correctly detect consumers. Cap workspace, sandbox, and service names at 19 characters to guarantee the combined DNS label (workspace--sandbox--service) fits within the 63-character RFC 1035 limit. Signed-off-by: Derek Carr <decarr@redhat.com>
…s-workspace collisions SandboxIndex keyed entries by sandbox name alone, causing same-named sandboxes in different workspaces to shadow each other in the in-memory index. Key by (workspace, name) tuple to match the persistence layer's scoping. Signed-off-by: Derek Carr <decarr@redhat.com>
…filters, TUI create exec, and CLI last-used sandbox - Thread workspace watch channel into PolicyLocalContext so SubmitPolicyAnalysis and GetDraftPolicy send the correct workspace for non-default workspace sandboxes. Gate denial and activity flushes on non-empty workspace to prevent silent data loss during initial settings poll race. - Fix three provider-profile scope filtering asymmetries: scope- mismatched providers now contribute their real token-grant bindings instead of being skipped or matched against the wrong profile; add symmetric profile_workspace filter in sandboxes_using_profile to prevent false "in use" blocks on workspace profile deletion. - TUI CreateResult now carries (name, workspace) so post-create exec targets the creation workspace, not the selected row. cycle_workspace resets current_workspace to "default" when entering all-workspaces mode to prevent silent cross-workspace creates. - CLI last-used sandbox is now workspace-keyed: save/load/clear store workspace alongside sandbox name and only match on the requested workspace. Signed-off-by: Derek Carr <decarr@redhat.com>
…in CLI create The GetProviderProfile pre-check in provider_create_with_options used the resource workspace instead of profile_workspace, so --global-profile validated against a workspace-scoped profile while binding the platform one. Signed-off-by: Derek Carr <decarr@redhat.com>
…list help text Signed-off-by: Derek Carr <decarr@redhat.com>
Include workspace field in sandbox and provider -o json/-o yaml output. Use workspace/name format in --names output under --all-workspaces. Parse --workspace from argv in tab completers so they complete names from the correct workspace instead of only reading the env var. Signed-off-by: Derek Carr <decarr@redhat.com>
Gate sandbox_from_object on the managed-by=openshell label so CRs written by other actors in the same namespace are silently skipped. Managed CRs missing required fields (orphans) log a warning instead of hard-erroring. Add managed-by label selector to list and watch ListParams so unmanaged CRs are filtered at the K8s API level. Callers treat Err as skip rather than fail, preventing one bad CR from breaking the entire list or watch stream. Signed-off-by: Derek Carr <decarr@redhat.com>
|
Follow-up to my earlier review comment. That pass reviewed the diff file-by-file; this pass audited the PR along four cross-cutting axes instead: (1) workspace provenance at every store call site, (2) workspace provenance at every client request-construction site, (3) the full cross-object reference graph under delete/recreate, and (4) a cross-layer behavior matrix (server/CLI/TUI/SDK/three drivers). Same scope as before: fresh install assumed, upgrade paths out of scope, missing authz deferred to Phase 2. The new findings are mostly relational — they live between components rather than inside any one file, which is why the first pass missed them. Additional must-fix candidates1. Watch-adoption path persists driver-supplied workspaces verbatim — one ghost record halts gateway-wide reconciliationThe create branch of
Consequences of the resulting Fix shape: normalize/validate the workspace at the adoption boundary (empty → 2. Inference routes reference providers by name with no delete guard and no cleanup — silent traffic rebinding
Secondary defect on the same edge: while the route dangles, the The codebase's own invariant elsewhere (the sandbox-attachment guard) is that name references must not dangle, so this is a gap, not name-as-identity by design. Fix shape: include inference routes in the provider delete guard (or cascade-delete them), and make 3. Provider deletion doesn't synchronize with attach — dangling attachment adopts a recreated provider's credentials
4. Service endpoints are never cleaned up on sandbox delete
Worth fixing, borderline blocking
Non-blocking notesDocs/help claim workspace names are DNS-1123/63 chars while the server enforces 19 ( Phase 2 readiness (informational — no action needed in this PR)Classified all 73 RPCs plus the data-plane paths for "can a membership check bolt on here." Good news: the What the audit confirmed cleanFor completeness, the coverage side: all 118 production store call sites classified (72 via |
russellb
left a comment
russellb
left a comment
There was a problem hiding this comment.
Automated workspace-scoping review of RFC 0011 Phase 1, focused on cross-workspace read/write confusion. Core control-plane read/write path scoped correctly; three actionable items below (one latent driver inconsistency + two hardening/upgrade notes).
| labels.insert(LABEL_SANDBOX_ID.to_string(), sandbox.id.clone()); | ||
| labels.insert(LABEL_SANDBOX_NAME.to_string(), sandbox.name.clone()); | ||
| labels.insert( | ||
| LABEL_SANDBOX_WORKSPACE.to_string(), |
There was a problem hiding this comment.
The openshell.ai/sandbox-workspace label is written here but is never used as a match criterion. find_managed_container_summary (~L1268) matches on namespace && id && name only, and require_sandbox_identifier still accepts name-only requests. This PR hardened the Podman and K8s drivers to require sandbox_id and look up exclusively by the globally-unique LABEL_SANDBOX_ID, but that fix was not mirrored here. All workspaces share one Docker namespace and name uniqueness is now only per-(workspace, name), so a name-only Get/Stop/Delete is workspace-ambiguous and .find() returns an arbitrary match — potentially operating on another workspace's same-named sandbox.
Latent today (the gateway always supplies sandbox_id), but the driver's gRPC contract still advertises name-only lookups as valid. Recommend mirroring the Podman/K8s hardening: require sandbox_id, or include LABEL_SANDBOX_WORKSPACE in the match predicate.
| /// * `Err(Conflict)` - Resource version mismatch (for `MatchResourceVersion`) | ||
| /// * `Err(UniqueViolation)` - Object already exists (for `MustCreate`) or name conflict | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub async fn put_if( |
There was a problem hiding this comment.
put_if is the production create/update path for every workspace-scoped resource (sandbox, provider, service, policy, inference route, ssh session), but unlike put_scoped_message and update_message_cas it does not enforce T::requires_workspace() against a non-empty workspace — and validate_object_metadata does not check workspace either. So the empty-workspace guard described in the RFC as living "in store write helpers" does not actually cover the primary write path.
Not reachable today because handlers resolve workspace to "default" before calling. Recommend adding the requires_workspace() check here (or in validate_object_metadata) so the invariant is enforced at the boundary rather than relying on every caller to resolve correctly.
| /// | ||
| /// Only `resource_version` is hydrated here; `workspace` is NOT backfilled from | ||
| /// the DB column because the workspace field is authoritative in the protobuf | ||
| /// payload at creation time. This is a breaking upgrade — pre-workspace records |
There was a problem hiding this comment.
Migration 006 backfills the workspace column to "default" for pre-upgrade rows, but because the payload is intentionally not backfilled here, a migrated record decodes with metadata.workspace == "". On resume this causes (a) SandboxIndex to key as ("", name) and miss "default" lookups, and (b) owned records written from that sandbox (e.g. SSH sessions in grpc/sandbox.rs) to be persisted with workspace "".
This falls within the accepted breaking-upgrade behavior, but the practical impact for anyone who resumes rather than recreates sandboxes is index/owned-record divergence — not just cosmetic empty fields. Worth an explicit "recreate sandboxes on upgrade" note in the upgrade docs.
- VM driver: propagate workspace in sandbox_snapshot so watch events round-trip the correct workspace back to the server. - Server: clean up service endpoints on sandbox delete, preventing orphaned endpoints from blocking workspace deletion. - TUI: accept workspace parameter from CLI instead of hardcoding "default", so --workspace and OPENSHELL_WORKSPACE are respected. - Python SDK: sync Sandbox context manager workspace from session after creation, so wait_ready/wait_deleted use the correct workspace when attaching via SandboxRef. - CLI: fix workspace name help text from 63 to 19 char limit. - RFC 0011: fix SSH alias format to match implementation. Signed-off-by: Derek Carr <decarr@redhat.com>
|
Feedback from my agent review [P1] Do not discard startup activity before workspace discoveryLocations:
The denial and activity aggregators drain a batch before invoking their flush callbacks. When the workspace is not yet known, these callbacks return successfully, so the drained events are permanently lost. This can drop policy proposals and activity telemetry during normal sandbox startup. Buffer the batch until workspace discovery completes, wait on the workspace receiver before starting the aggregators, or return a retryable error so the aggregator retains the batch. [P1] Preserve global profile scope throughout provider creationLocation:
Pass [P1] Prevent resource creation from racing workspace deletionLocation:
Use a persisted deleting state checked by resource creation, transactional exclusion, or another mechanism that closes the check/delete race. [P2] Paginate sandbox-owned service cleanupLocation: The new service-endpoint cleanup lists only the first 1,000 endpoints in the workspace and then deletes the sandbox. A sandbox whose endpoints fall after that page is deleted while its endpoints remain. Those orphaned records then block workspace deletion and can be presented as belonging to a later same-named sandbox. Query endpoints by their persisted sandbox label, paginate until exhaustion, or store them under a sandbox-owned scope that supports [P2] Keep Python workspace synchronization test-cleanLocations:
The new unconditional Only resynchronize the workspace on the [P2] Scope TUI profile joins by workspaceLocations:
The all-workspaces provider view loads profiles only from Resolve profiles by [P2] Give all-workspace pagination a total orderingLocations:
Cross-workspace queries order results only by Add [P2] Persist workspace labels in the selector indexLocation:
Serialize the already-validated workspace labels into the persistence label column before calling [P2] Apply the new workspace-routing limit to generated sandbox namesLocations:
This PR lowers the explicit sandbox-name limit from 253 to 19 characters so Validate the generated name and regenerate or fall back until it satisfies the new routable-name constraint. [P2] Require managed-resource labels in driver ID lookupsLocations:
Several get, existence, and deletion paths filter only by the OpenShell sandbox ID. They can select an unmanaged platform object carrying the same label, and deletion may act on that object. Include the OpenShell managed-by label in every lookup, not only list/watch paths. Lower-priority feedback[P2] Enforce persistence workspace invariantsLocation: The new raw [P3] Complete the global-profile CLI surface
[P3] Qualify platform-profile dependency diagnosticsGlobal-profile validation and deletion diagnostics identify dependent sandboxes only by name and then deduplicate those names. For example, dependencies on both |
5 participants
Summary
Implements Phase 1 of RFC 0011 — the workspace and membership model that provides hard isolation boundaries for multi-player OpenShell deployments.
delete_by_scopestore method for sandbox-owned record cleanup.resolve_profile_workspacepreserves empty string for platform scope whileresolve_workspacenormalizes to "default". Platform-profile attachment checks scan all workspaces to detect consumers. Scope-mismatched providers contribute their real token-grant bindings for ambiguity detection instead of being skipped or matched against the wrong profile scope.{workspace}--{sandbox}--{service}.{domain}format for all workspaces including default. Workspace, sandbox, and service names capped at 19 characters to guarantee the combined DNS label fits within the 63-character RFC 1035 limit.sandbox_snapshotpropagates workspace so watch events round-trip the correct workspace back to the server.sandbox_from_objectgates onmanaged-by=openshelllabel — CRs written by other actors in the namespace are silently skipped. Managed CRs missing required fields (orphans) log a warning and are skipped rather than hard-erroring. Label selector added to list and watchListParamsso unmanaged CRs are filtered at the K8s API level.is_openshell_managedhelper centralizes the ownership check.OPENSHELL_WORKSPACEenv var. WORKSPACE column as first column in list outputs when --all-workspaces (matching kubectl --all-namespaces convention);-o json/-o yamloutput always includesworkspacefield;--namesoutput usesworkspace/nameformat under--all-workspaces(matching kubectl-o name --all-namespaces). Workspace CRUD and membership subcommands; inference help text updated from "gateway-level" to "workspace-level". Last-used sandbox fallback keyed by (gateway, workspace) — saves workspace alongside sandbox name and only matches the requested workspace, preventing cross-workspace mis-targeting.--workspace/OPENSHELL_WORKSPACE(no longer hardcoded to "default"). Selection indices reset on workspace cycle. Post-create exec uses the creation workspace (carried in CreateResult) instead of the selected row's workspace. cycle_workspace resets current_workspace to "default" when entering all-workspaces mode to prevent silent cross-workspace creates.wait_ready/wait_deleteduse the correct workspace when attaching viaSandboxRef).Breaking changes
{workspace}--{sandbox}.{domain}(previously{sandbox}.{domain}). The default workspace does not preserve the old format.openshell.ai/sandbox-workspacelabel for workspace tracking. This is intentional — pre-existing containers from before the upgrade will not be visible to the watcher and require a clean shutdown before upgrade.openshell-{name}.{workspace}(previouslyopenshell-{name}). Old entries are not automatically cleaned up.workspacekeyword argument with no default — this is intentional to make workspace selection explicit. The CLI defaults to"default"via--workspace, but the SDK requires callers to be explicit about which workspace they operate in.SetClusterInferenceConfig→SetInferenceRoute,GetClusterInferenceConfig→GetInferenceRoute,ClusterInferenceConfig→InferenceRouteConfig. This is a wire-breaking change — the inference route API is pre-GA with no deployed consumers of the old RPC names.workspace\nname. Old single-line files are ignored on load (user re-selects on first use after upgrade).Related Issue
#1977
Changes
Testing
Known remaining work (Phase 2)
Non-blocking follow-ups noted in review (no action needed for this PR)
--workspacedefaults to"default"client-side; consider removing the default and relying on server-sideresolve_workspacenormalization, with client-side operations (last-sandbox, completers) resolving workspace from server responses.unmapped("workspace", ...)as part of a broader workspace attribution pass across all OCSF event sitessandbox_sync_guard— attach racing delete can leave a dangling by-name attachment (pre-existing TOCTOU)cleanup_sandbox_ssh_sessionsreads a single page of 1000 with no pagination loopChecklist